Automated Test: enhance-passkey-authentication-flow #321
Conversation
closes #41242 closes #41008 Signed-off-by: mposolda <mposolda@gmail.com>
This change modifies the method signature to require a UserModel parameter for proper user context validation during conditional passkey checks.
📝 WalkthroughWalkthroughThis pull request refactors WebAuthn and passkey authentication handling in Keycloak by extracting reusable helper methods for reauthentication logic, making the reauthentication flag public, and introducing comprehensive test coverage for passwordless reauthentication scenarios with discoverable and external keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (1)
336-421:⚠️ Potential issue | 🟡 MinorFix typo on Line 413: "Succesful" → "Successful".
The event handling pattern in this test is correct. The LOGIN event generated at line 363 is explicitly cleared by
events.clear()on line 383 before the next sequence of assertions begins. This reset-via-clear approach is intentional and safe—expectLogin().assertEvent()does consume (remove) events from the queue viapoll().
🤖 Fix all issues with AI agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java`:
- Line 47: The call to isConditionalPasskeysEnabled() in UsernameForm is missing
the required UserModel argument; update the condition in the UsernameForm class
to call isConditionalPasskeysEnabled(context.getUser()) (matching how
UsernamePasswordForm uses it) so the method receives the current user from
context.getUser().
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java`:
- Around line 136-141: The error challenge path currently calls
isConditionalPasskeysEnabled(context.getUser()) which is false when
context.getUser() is null, removing passkey UI after failed username submission;
update challenge(AuthenticationFlowContext context, String error, String field)
to fall back when context.getUser() is null by reading the attempted username
from the authentication session (e.g.
context.getAuthenticationSession().getAuthNote("username") or the equivalent
constant), resolve the UserModel via
context.getSession().users().getUserByUsername(context.getRealm(),
attemptedUsername), then call isConditionalPasskeysEnabled(resolvedUser) and if
true invoke webauthnAuth.fillContextForm(context) before returning
super.challenge(...).
- Around line 113-117: The call to isConditionalPasskeysEnabled() in
UsernameForm has the wrong signature; update the call to pass the current user
and guard for null: use context.getUser() in the condition (check
context.getUser() != null) and call
isConditionalPasskeysEnabled(context.getUser()) (negate as intended in the
original conditional) so the UsernameForm call matches the protected boolean
isConditionalPasskeysEnabled(UserModel user) in UsernamePasswordForm.
🧹 Nitpick comments (3)
testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.java (1)
261-302: Consider extracting shared setup withwebauthnLoginWithDiscoverableKey.Lines 261–302 largely duplicate the setup and first login from
webauthnLoginWithDiscoverableKey(Lines 112–153). A shared helper (e.g.,loginWithDiscoverableKeySetup()) returning theUserRepresentationcould reduce duplication across both tests.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysOrganizationAuthenticationTest.java (1)
291-296: Nit: consider assertingloginPage.assertCurrent()or WebAuthn page state before the auto-login.Between
oauth.openLoginForm()(Line 292) andappPage.assertCurrent()(Line 295), the discoverable key auto-authenticates so there's no visible intermediate page. This is fine for the happy path, but if the auto-login silently fails in the future, the test will produce a confusingappPage.assertCurrent()failure with no indication of what page was actually displayed. A brief comment or a try-catch with a diagnostic message could help future debugging — though this is a very minor point.testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernamePasswordFormTest.java (1)
291-296: Element-absence checks are overly complex and catch too broadly.The current pattern has unnecessary complexity:
driver.findElement()never returnsnull, soassertThat(..., nullValue())always fails if the element is found (throwingAssertionError, anError, notException). If the element is absent,findElementthrowsNoSuchElementExceptionbefore the assertion is reached.- Catching
Exceptionis too broad — it would silently swallow unexpected exceptions (e.g.,StaleElementReferenceException,WebDriverException).The idiomatic Selenium pattern is simpler:
♻️ Suggested simplification (apply to both blocks at lines 291-296 and 312-317)
- try { - MatcherAssert.assertThat(driver.findElement(By.xpath("//form[`@id`='webauth']")), nullValue()); - fail("Not expected to have webauthn button"); - } catch (Exception nsee) { - // expected - } + try { + driver.findElement(By.xpath("//form[`@id`='webauth']")); + fail("Not expected to have webauthn button"); + } catch (NoSuchElementException nsee) { + // expected + }This catches only the expected
NoSuchElementException, andfail()is reached only if the element is unexpectedly present.Also applies to: 312-317
| @@ -44,7 +44,7 @@ public UsernameForm(KeycloakSession session) { | |||
|
|
|||
| @Override | |||
| public void authenticate(AuthenticationFlowContext context) { | |||
| if (context.getUser() != null) { | |||
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all definitions and usages of isConditionalPasskeysEnabled across the codebase
rg -nP 'isConditionalPasskeysEnabled\s*\(' --type=java -B2 -A2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 3116
Compilation error: isConditionalPasskeysEnabled() called without required UserModel argument.
The parent class UsernamePasswordForm defines isConditionalPasskeysEnabled(UserModel user) at line 160, but line 47 invokes it without arguments. This will fail to compile. Based on correct usage patterns in UsernamePasswordForm at lines 115 and 137, the call should pass context.getUser().
Proposed fix
- if (context.getUser() != null && !isConditionalPasskeysEnabled()) {
+ if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (context.getUser() != null && !isConditionalPasskeysEnabled()) { | |
| if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser())) { |
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java`
at line 47, The call to isConditionalPasskeysEnabled() in UsernameForm is
missing the required UserModel argument; update the condition in the
UsernameForm class to call isConditionalPasskeysEnabled(context.getUser())
(matching how UsernamePasswordForm uses it) so the method receives the current
user from context.getUser().
| } | ||
| // setup webauthn data when passkeys enabled | ||
| if (isConditionalPasskeysEnabled(context.getUser())) { | ||
| webauthnAuth.fillContextForm(context); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for ENABLE_WEBAUTHN_CONDITIONAL_UI usage
echo "=== ENABLE_WEBAUTHN_CONDITIONAL_UI usage ==="
rg -n 'ENABLE_WEBAUTHN_CONDITIONAL_UI' --type=java -C 2
echo ""
echo "=== isConditionalPasskeysEnabled usage ==="
rg -n 'isConditionalPasskeysEnabled' --type=java -C 2
echo ""
echo "=== fillContextForm usage in relevant files ==="
rg -n 'fillContextForm' --type=java -C 2 | head -100Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 11070
🏁 Script executed:
#!/bin/bash
# Find the WebAuthnConditionalUIAuthenticator and WebAuthnAuthenticator files
echo "=== Finding WebAuthnConditionalUIAuthenticator ==="
fd -t f -n 'WebAuthnConditionalUIAuthenticator' --type=java
echo ""
echo "=== Finding WebAuthnAuthenticator ==="
fd -t f -n 'WebAuthnAuthenticator' --type=javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 655
🏁 Script executed:
#!/bin/bash
# Look for the test file mentioned
echo "=== Finding PasskeysUsernameFormTest ==="
fd -t f -n 'PasskeysUsernameFormTest'
echo ""
echo "=== Finding UsernamePasswordForm ==="
fd -t f -n 'UsernamePasswordForm'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Find and view UsernameForm.java
rg -l 'class UsernameForm' --type=javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 277
🏁 Script executed:
#!/bin/bash
# View the content of UsernameForm.java
rg -A 100 'public class UsernameForm' --type=java | head -150Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 9325
🏁 Script executed:
#!/bin/bash
# Check what isConditionalPasskeysEnabled methods exist in UsernameForm
rg -B2 -A5 'isConditionalPasskeysEnabled' services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 512
🏁 Script executed:
#!/bin/bash
# Find and view test file to understand which authenticator is tested
find . -name '*PasskeysUsernameFormTest*' -type fRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 221
🏁 Script executed:
#!/bin/bash
# View full UsernameForm.java
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 5223
🏁 Script executed:
#!/bin/bash
# View PasskeysUsernameFormTest.java
cat -n ./testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/webauthn/passwordless/PasskeysUsernameFormTest.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 17092
🏁 Script executed:
#!/bin/bash
# Search for all isConditionalPasskeysEnabled method definitions
echo "=== All isConditionalPasskeysEnabled methods ==="
rg -n 'protected.*boolean.*isConditionalPasskeysEnabled' --type=java -A 2
echo ""
echo "=== Parent class of UsernamePasswordForm ==="
rg -n 'public class UsernamePasswordForm' --type=java -A 5Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2403
🏁 Script executed:
#!/bin/bash
# Check UsernamePasswordForm full authenticate method to trace user handling
rg -n 'public void authenticate\|if \(context.getUser' services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java -A 1 | head -40Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Search for how validateUser might set context user
rg -n 'validateUser\|context.setUser' services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java -B 2 -A 2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 69
🏁 Script executed:
#!/bin/bash
# Find AbstractUsernameFormAuthenticator
fd -t f 'AbstractUsernameFormAuthenticator' --type=javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 258
🏁 Script executed:
#!/bin/bash
# View AbstractUsernameFormAuthenticator
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/AbstractUsernameFormAuthenticator.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 13732
🏁 Script executed:
#!/bin/bash
# Search for isConditionalPasskeysEnabled() with no parameters
rg -n 'isConditionalPasskeysEnabled\(\)' --type=java -B 2 -A 2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 914
🏁 Script executed:
#!/bin/bash
# Search more thoroughly for all isConditionalPasskeysEnabled methods in the codebase
rg 'isConditionalPasskeysEnabled' -B 1 -A 2Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2605
🏁 Script executed:
#!/bin/bash
# Check the full authenticate method of UsernamePasswordForm
wc -l services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java | sed -n '80,125p'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 2675
🏁 Script executed:
#!/bin/bash
# Check if there's a parameterless isConditionalPasskeysEnabled in UsernameForm itself
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java | grep -A 5 'isConditionalPasskeysEnabled'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 474
🏁 Script executed:
#!/bin/bash
# Check if there's an override in UsernameForm for the method
rg -n 'class UsernameForm' -A 70 services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.java | grep -i 'conditional'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 165
🏁 Script executed:
#!/bin/bash
# Try to compile or check for compilation errors - search for all methods in UsernameForm
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernameForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 5223
🏁 Script executed:
#!/bin/bash
# Final verification: search for any parameterless isConditionalPasskeysEnabled definition
rg 'protected.*boolean.*isConditionalPasskeysEnabled\(\)' --type fRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check if there's any method added in the changed lines that might define a parameterless version
cat -n services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java | sed -n '150,164p'Repository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 547
🏁 Script executed:
#!/bin/bash
# Verify the entire UsernamePasswordForm class to see all method definitions
wc -l services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java
echo ""
echo "=== Checking for all isConditionalPasskeysEnabled definitions in the entire file ==="
grep -n 'isConditionalPasskeysEnabled' services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.javaRepository: admin-coderabbit/coderabbit-keycloak-mirror
Length of output: 483
Method signature mismatch prevents compilation: isConditionalPasskeysEnabled() in UsernameForm requires a parameter.
Line 47 of UsernameForm calls isConditionalPasskeysEnabled() with no arguments, but the only definition in UsernamePasswordForm at line 160 is protected boolean isConditionalPasskeysEnabled(UserModel user) which requires a UserModel parameter. UsernameForm does not override this method, so the call will fail to compile.
Change line 47 in UsernameForm to: if (context.getUser() != null && !isConditionalPasskeysEnabled(context.getUser()))
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java`
around lines 113 - 117, The call to isConditionalPasskeysEnabled() in
UsernameForm has the wrong signature; update the call to pass the current user
and guard for null: use context.getUser() in the condition (check
context.getUser() != null) and call
isConditionalPasskeysEnabled(context.getUser()) (negate as intended in the
original conditional) so the UsernameForm call matches the protected boolean
isConditionalPasskeysEnabled(UserModel user) in UsernamePasswordForm.
| protected Response challenge(AuthenticationFlowContext context, String error, String field) { | ||
| if (context.getUser() == null && webauthnAuth != null && webauthnAuth.isPasskeysEnabled()) { | ||
| // setup webauthn data when the user is not already selected | ||
| if (isConditionalPasskeysEnabled(context.getUser())) { | ||
| // setup webauthn data when possible | ||
| webauthnAuth.fillContextForm(context); | ||
| } | ||
| return super.challenge(context, error, field); |
There was a problem hiding this comment.
Same concern applies to the error challenge path.
When challenge(context, error, field) is invoked after a failed login attempt where the user was not identified, context.getUser() will be null, and isConditionalPasskeysEnabled(null) returns false. The passkey conditional UI won't be repopulated on the error form, removing the passkey option after a failed username submission.
🤖 Prompt for AI Agents
In
`@services/src/main/java/org/keycloak/authentication/authenticators/browser/UsernamePasswordForm.java`
around lines 136 - 141, The error challenge path currently calls
isConditionalPasskeysEnabled(context.getUser()) which is false when
context.getUser() is null, removing passkey UI after failed username submission;
update challenge(AuthenticationFlowContext context, String error, String field)
to fall back when context.getUser() is null by reading the attempted username
from the authentication session (e.g.
context.getAuthenticationSession().getAuthNote("username") or the equivalent
constant), resolve the UserModel via
context.getSession().users().getUserByUsername(context.getRealm(),
attemptedUsername), then call isConditionalPasskeysEnabled(resolvedUser) and if
true invoke webauthnAuth.fillContextForm(context) before returning
super.challenge(...).
This pull request was automatically created by
@coderabbitai/e2e-reviewer.Batch created pull request.
Summary by CodeRabbit
New Features
Bug Fixes
Tests